fix #857 - fix on put when both @id and id are present by Simperfit · Pull Request #877 · api-platform/core

Simperfit · 2016-12-10T11:28:13Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#857
License	MIT
Doc PR

Add an non-regression test and modify the comportement of itemNormalizer

dunglas · 2016-12-10T12:43:23Z

    {
        // Avoid issues with proxies if we populated the object
-        if (isset($data['id']) && !isset($context['object_to_populate'])) {
+        if (isset($data['@id']) && !isset($context['object_to_populate'])) {


This snippet already exists in the JSON-LD normalizer: https://github.com/api-platform/core/blob/master/src/JsonLd/Serializer/ItemNormalizer.php#L93

@id is specific to JSON-LD, so it should not be here. However id can contain an IRI too (when not using JSON-LD). So it should be kept. I guess it's a priority problem, in https://github.com/api-platform/core/blob/master/src/JsonLd/Serializer/ItemNormalizer.php#L101 the call to the parent method should be done before.

Simperfit · 2016-12-10T16:20:47Z

@dunglas This is now working but i'm not sure about the fix, since I don't think it's in the same scope then the iriConverter is, clearly this is not an IRI, do you have any thing in mind that would be better ?

EDIT: it's not the right fix ;)

Simperfit · 2016-12-10T19:55:08Z

Again, maybe not the best fixes, but better than before since it's working and it does not break anything.

dunglas · 2016-12-13T16:23:02Z

            }
-
-            $context['object_to_populate'] = $this->iriConverter->getItemFromIri($data['id'], $context + ['fetch_data' => false]);
+            $context['object_to_populate'] = $this->iriConverter->getItemFromIri(is_int($data['id']) ? $this->iriConverter->getIriFromResourceClass($context['resource_class']).'/'.$data['id'] : $data['id'], $context + ['fetch_data' => false]);


id can be from other types (uuid are strings for instance).

What could I do about that ?

Edit: Was thinking about try catching and doing the getIriFromResourceClass, WDYT @dunglas ?

dunglas

When test will be green.

theofidry

See my comments for some nitpicking changes, 👍 overall :)

theofidry · 2016-12-19T19:31:28Z

+ * @ApiResource(attributes={
+ *     "normalization_context"={"groups"={"related_output", "output"}},
+ *     "denormalization_context"={"groups"={"related_input", "input"}}
+ *


useless extra line

theofidry · 2016-12-19T19:31:51Z

+    /**
+     * @return int
+     */
+    public function getId()


wouldn't a return typehint more suitable here and the other places in the same class?

teohhanhui · 2016-12-20T03:34:28Z

    }
    """

+  Scenario: Create a resource with relation


Please fix the scenario summary (same for all below)...

teohhanhui · 2016-12-20T03:36:52Z

The fix is simple: We must not support embedded relations with application/json, because the semantics are undefined. We have had this discussion before on Slack, and I thought a decision was made...

So I'm -1 since this is the wrong fix.

/cc @api-platform/core-team

dunglas · 2016-12-20T09:12:06Z

@teohhanhui the issue here isn't with embedded relations but using raw ids instead of IRIs in raw JSON. Not sure we should support that too.

teohhanhui · 2016-12-20T11:37:46Z

What is the issue with deserializing using a raw id property (or composite id represented as separate properties)? I think it should be supported.

Simperfit · 2016-12-22T09:50:01Z

@teohhanhui updated

teohhanhui · 2016-12-27T04:47:03Z

+            try {
+                $context['object_to_populate'] = $this->iriConverter->getItemFromIri($data['id'], $context + ['fetch_data' => false]);
+            } catch (InvalidArgumentException $e) {
+                $context['object_to_populate'] = $this->iriConverter->getItemFromIri($this->iriConverter->getIriFromResourceClass($context['resource_class']).'/'.$data['id'], $context + ['fetch_data' => false]);


How is this going to work with composite identifiers?

And also this is hardcoded to use the property "id"? 😢

@teohhanhui @dunglas do we want to support composite identifiers ?

Simperfit · 2017-01-03T08:56:05Z

ping @teohhanhui @dunglas modification done, WDYT ?

dunglas · 2017-01-03T10:02:34Z

+                $context['object_to_populate'] = $this->iriConverter->getItemFromIri($data['id'], $context + ['fetch_data' => false]);
+            } catch (InvalidArgumentException $e) {
+                foreach ($this->propertyNameCollectionFactory->create($context['resource_class'], $context) as $propertyName) {
+                    if (true === $this->propertyMetadataFactory->create($context['resource_class'], $propertyName)->isIdentifier()) {


If we don't support yet composite identifiers, you can break after the first find. Or you can add support for composite identifiers :)

+1 for the composite support :D

I think for now we should bug fix this and then add that feature ?

dunglas · 2017-01-03T10:04:10Z

+                        $identifier = $propertyName;
+                    }
+                }
+                $context['object_to_populate'] = $this->iriConverter->getItemFromIri($this->iriConverter->getIriFromResourceClass($context['resource_class']).'/'.$data[$identifier], $context + ['fetch_data' => false]);


I would use sprintf here for clarity.

dunglas · 2017-01-25T09:51:57Z

@api-platform/core-team is everyone OK to merge this one too? I want to publish a bugfix release today.

dunglas · 2017-01-25T13:47:23Z

Thanks @Simperfit!

@id

fix api-platform#857 - fix on put when both @id and id are present

Simperfit force-pushed the fix/857-embedded branch from c84c974 to 1caa29c Compare December 10, 2016 11:29

dunglas requested changes Dec 10, 2016

View reviewed changes

Simperfit force-pushed the fix/857-embedded branch from 769085e to d17eb1f Compare December 10, 2016 14:38

Simperfit force-pushed the fix/857-embedded branch from 4962c1f to ae817d5 Compare December 10, 2016 16:23

Simperfit changed the title ~~fix #857 - fix on put when both @id and id are present~~ [WIP] fix #857 - fix on put when both @id and id are present Dec 10, 2016

Simperfit force-pushed the fix/857-embedded branch from 9c730f2 to d644132 Compare December 10, 2016 19:52

Simperfit changed the title ~~[WIP] fix #857 - fix on put when both @id and id are present~~ fix #857 - fix on put when both @id and id are present Dec 10, 2016

dunglas requested changes Dec 13, 2016

View reviewed changes

Simperfit force-pushed the fix/857-embedded branch from 4016c69 to 00b4d93 Compare December 19, 2016 13:42

dunglas approved these changes Dec 19, 2016

View reviewed changes

Simperfit force-pushed the fix/857-embedded branch from 00b4d93 to 3cfd553 Compare December 19, 2016 15:53

Simperfit requested review from teohhanhui and theofidry December 19, 2016 17:10

theofidry approved these changes Dec 19, 2016

View reviewed changes

teohhanhui suggested changes Dec 20, 2016

View reviewed changes

Simperfit force-pushed the fix/857-embedded branch from 3cfd553 to 6244be0 Compare December 22, 2016 09:49

dunglas added the bug label Dec 23, 2016

teohhanhui suggested changes Dec 27, 2016

View reviewed changes

Simperfit force-pushed the fix/857-embedded branch from 5402a00 to a76f79f Compare December 27, 2016 18:48

dunglas requested changes Jan 3, 2017

View reviewed changes

Simperfit force-pushed the fix/857-embedded branch from c1a931a to 0cb7e08 Compare January 7, 2017 17:20

soyuka approved these changes Jan 25, 2017

View reviewed changes

fix api-platform#857 - fix on put when both @id and id are present

137a36e

dunglas force-pushed the fix/857-embedded branch from 0cb7e08 to 137a36e Compare January 25, 2017 13:41

dunglas approved these changes Jan 25, 2017

View reviewed changes

dunglas merged commit 01303fa into api-platform:2.0 Jan 25, 2017

Simperfit deleted the fix/857-embedded branch January 25, 2017 15:01

magarzon pushed a commit to magarzon/core that referenced this pull request Feb 12, 2017

Merge pull request api-platform#877 from Simperfit/fix/857-embedded

6f32537

fix api-platform#857 - fix on put when both @id and id are present

teohhanhui mentioned this pull request Feb 16, 2017

Deserialization bug on background job (IriConverter) #946

Closed

teohhanhui mentioned this pull request Mar 9, 2017

Use plain IDs api-platform/api-platform#249

Closed

bendavies mentioned this pull request Jun 19, 2018

Fix allow creating resources with custom identifers #2022

Closed

Uh oh!

Uh oh!

Conversation

Simperfit commented Dec 10, 2016

Uh oh!

dunglas Dec 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Simperfit commented Dec 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Simperfit commented Dec 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Simperfit Dec 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas left a comment

Choose a reason for hiding this comment

Uh oh!

theofidry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teohhanhui commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dunglas commented Dec 20, 2016

Uh oh!

teohhanhui commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Simperfit commented Dec 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Simperfit commented Jan 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas commented Jan 25, 2017

Uh oh!

dunglas commented Jan 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dunglas Dec 10, 2016 •

edited

Loading

Simperfit commented Dec 10, 2016 •

edited

Loading

Simperfit Dec 13, 2016 •

edited

Loading

teohhanhui commented Dec 20, 2016 •

edited

Loading

teohhanhui commented Dec 20, 2016 •

edited

Loading